Fixing incorrect variable name 'size_gb' which actually represents 'size_mb' by SRIKKANTH · Pull Request #4409 · microsoft/lisa

SRIKKANTH · 2026-04-06T16:11:09Z

Description

Fixing incorrect variable name 'size_gb' which actually represents 'size_mb'

Related Issue

size_gb is misleading variable name.

Type of Change

Checklist

Description is filled in above
No credentials, secrets, or internal details are included
Peer review requested (if not, add required peer reviewers after raising PR)
Tests executed and results posted below

Test Validation

Key Test Cases:
perf_nvme
perf_premium_datadisks_4k
perf_premium_datadisks_1024k

Impacted LISA Features:
None

Tested Azure Marketplace Images:

canonical ubuntu-24_04-lts server latest

Test Results

Image	VM Size	Result
canonical ubuntu-24_04-lts server latest	Standard_D32s_v5	PASSED

Copilot

Pull request overview

This PR corrects a misleading fio API parameter name (size_gb) that actually represented a MiB value, and updates several test suites to pass sizes using the corrected size_mb keyword.

Changes:

Renamed fio size parameter from size_gb to size_mb and updated fio command construction accordingly.
Updated storage/power/cpu/performance test call sites to use size_mb (and in some cases convert GB → MB explicitly).

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
lisa/tools/fio.py	Renames fio size parameter to `size_mb` and uses it to build `--size=<n>M`.
lisa/microsoft/testsuites/storage/storagesuite.py	Updates fio call to use `size_mb` with a GB→MB conversion.
lisa/microsoft/testsuites/power/common.py	Updates fio call to use `size_mb` with a GB→MB conversion.
lisa/microsoft/testsuites/performance/storageperf.py	Changes NFS fio size to `256 * 1024` MB (substantial workload change).
lisa/microsoft/testsuites/performance/common.py	Fixes fio invocation to pass `size_mb` using the correct keyword.
lisa/microsoft/testsuites/cpu/cpusuite.py	Updates async fio call to use `size_mb` with a GB→MB conversion.

Comments suppressed due to low confidence (1)

lisa/tools/fio.py:103

Renaming the public keyword parameter from size_gb to size_mb is a breaking API change for any out-of-tree callers that still pass size_gb= (or rely on the previous positional argument order). Consider keeping backward compatibility by accepting both names (e.g., a deprecated size_gb alias that maps to size_mb) for at least one release cycle, or raising a targeted, actionable error when size_gb is provided.

    def launch(
        self,
        name: str,
        filename: str,
        mode: str,
        iodepth: int,
        numjob: int,
        time: int = 120,
        ssh_timeout: int = 6400,
        block_size: str = "4K",
        size_mb: int = 0,
        direct: bool = True,
        gtod_reduce: bool = False,
        group_reporting: bool = True,
        overwrite: bool = False,

Copilot · 2026-04-06T16:16:17Z

        time: int = 120,
        ssh_timeout: int = 6400,
        block_size: str = "4K",
-        size_gb: int = 0,
+        size_mb: int = 0,
        direct: bool = True,
        gtod_reduce: bool = False,


PR hygiene: the PR is marked as a bug fix but the description lists no related issue. Consider linking an issue (or explicitly stating why none exists) for traceability.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Copilot · 2026-04-06T16:51:58Z

                    time=0,
                    block_size="",
-                    size_gb=100,
+                    size_mb=100,  # Keep fio data size at 100 MiB to bound test runtime.
                    group_reporting=False,
                    do_verify=True,


verify_disk_with_fio_verify_option runs fio in a for _ in range(100) loop with time=0 (no --runtime) and size_mb=100 * 1024, so each iteration will attempt to write+verify ~100 GiB and the loop can generate ~10 TiB of IO. This is likely to make the functional test impractically slow and/or hit CI timeouts. Consider keeping the previous effective size (100 MiB), or switching to time_based=True with an explicit runtime, or reducing the iteration count/size with a clear rationale. Also the inline comment says “100GB” but the expression is 100*1024 MiB (GiB).

Copilot · 2026-04-06T16:51:59Z

        block_size="1M",
        overwrite=True,
-        size_gb=1,
+        size_mb=1024,  # 1GB


The new size_mb=1024 comment says “1GB”, but fio’s --size=<n>M uses MiB units. If the intent is 1 GiB this is fine; otherwise consider either using 1000 MiB for 1 GB (decimal) or updating the comment/variable naming to avoid unit confusion (GB vs GiB).

Suggested change

size_mb=1024, # 1GB

size_mb=1024, # 1 GiB

Copilot · 2026-04-06T16:51:59Z

                time=fio_run_time,
                block_size="1M",
-                size_gb=fio_data_size_in_gb,
+                size_mb=fio_data_size_in_gb * 1024,


fio_data_size_in_gb is multiplied by 1024 to produce size_mb, which corresponds to GiB→MiB conversion. Consider adding a short inline comment or renaming the variable to avoid confusion between GB (decimal) vs GiB (binary) units, since fio’s --size=<n>M is MiB-based.

Suggested change

size_mb=fio_data_size_in_gb * 1024,

size_mb=fio_data_size_in_gb * 1024, # fio --size=<n>M uses MiB; convert GiB to MiB.

Copilot · 2026-04-06T16:51:59Z

    def launch(
        self,
        name: str,
        filename: str,
        mode: str,
        iodepth: int,
        numjob: int,
        time: int = 120,
        ssh_timeout: int = 6400,
        block_size: str = "4K",
-        size_gb: int = 0,
+        size_mb: int = 0,
        direct: bool = True,
        gtod_reduce: bool = False,


Renaming the public Fio.launch(...)/launch_async(...)/_get_command(...) parameter from size_gb to size_mb is a breaking change for any out-of-tree callers using the old keyword. If backward compatibility is desired, consider temporarily supporting both names (e.g., keep size_gb as a deprecated alias that maps to size_mb with a warning) and remove it in a later major release.

…ize_mb' Fixing incorrect variable name 'size_gb' which actually represents 'size_mb' Co-Authored-By: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Copilot · 2026-04-06T17:39:31Z

+        fio_data_size_in_mb = 1
        try:
-            image_folder_path = node.find_partition_with_freespace(fio_data_size_in_gb)
+            image_folder_path = node.find_partition_with_freespace(fio_data_size_in_mb)
            # Each CPU takes ~10 seconds to toggle offline-online


node.find_partition_with_freespace takes size_in_gb, but this call now passes fio_data_size_in_mb. That makes the unit semantics incorrect (the value will be treated as GiB), and could cause unexpected failures or over-strict disk selection if fio_data_size_in_mb is adjusted. Consider keeping a separate required_space_gb variable (or converting MB→GB) when calling find_partition_with_freespace.

Copilot · 2026-04-06T17:39:31Z

+        fio_data_size_in_mb = 1
        try:
-            image_folder_path = node.find_partition_with_freespace(fio_data_size_in_gb)
+            image_folder_path = node.find_partition_with_freespace(fio_data_size_in_mb)
            # Each CPU takes ~10 seconds to toggle offline-online
            fio_run_time = 300 + (node.tools[Lscpu].get_thread_count() * 10)
            fio_process = node.tools[Fio].launch_async(


The constants fio_data_size_in_mb = 1 and the base 300 seconds in fio_run_time = 300 + (...) are magic numbers that control test behavior but aren’t explained inline. Please add brief rationale/comments (or named constants) so future changes don’t accidentally skew workload size or runtime assumptions.

Copilot · 2026-04-06T17:39:32Z

    def launch(
        self,
        name: str,
        filename: str,
        mode: str,
        iodepth: int,
        numjob: int,
        time: int = 120,
        ssh_timeout: int = 6400,
        block_size: str = "4K",
-        size_gb: int = 0,
+        size_mb: int = 0,
        direct: bool = True,


This PR is labeled as a bug fix, but the PR description lists no related issue. Consider linking the related issue (or a tracking work item) for traceability, especially since this changes a public API keyword argument on Fio.launch/launch_async.

LiliDeng · 2026-04-07T02:17:21Z

-        if size_gb:
-            cmd += f" --size={size_gb}M"
+        if size_mb:
+            cmd += f" --size={size_mb}M"


just change this into cmd += f" --size={size_gb}G" should be enough.

If we do this all fio tests will run against GB ( * 1024 ) of current values. Which will result in timeouts as the time required for fio file size in GBs will be more than MBs.
For example some tests use size_gb=8192 which will be 8TB instead of current test file size 8GB.
One of the NFS testcase uses 100MB now in a loop of 100 iteration. With this suggestion it will become 100 GB * 100 = 10TB data reads and writes.

Please check the original case parameters

"perf_disk()" accepts the value as 'size_mb' but passing the value to "fio.launch()" as 'size_gb'.
Please check: https://github.com/microsoft/lisa/blob/main/lisa/microsoft/testsuites/performance/common.py#L159

This is a typo issue not a test case issue.

If you want to run current fio tests against 1023GB like we had in lisav2, I can raise a different PR for that after through testing. This is for just fixing the typo which impacts testcases not just disk perf tests but CPU and Power test cases as well.

LiliDeng · 2026-04-10T01:03:58Z

+        fio_data_size_in_mb = 1
        try:
-            image_folder_path = node.find_partition_with_freespace(fio_data_size_in_gb)
+            image_folder_path = node.find_partition_with_freespace(fio_data_size_in_mb)


the unit of this method find_partition_with_freespace is gb

LiliDeng · 2026-04-10T01:10:41Z

-        if size_gb:
-            cmd += f" --size={size_gb}M"
+        if size_mb:
+            cmd += f" --size={size_mb}M"


@SRIKKANTH thanks for fixing this, but I still would like to just change cmd += f" --size={size_gb}M" into cmd += f" --size={size_gb}G"

Let me put the PR in draft until I finish the validation with you suggestion.

SRIKKANTH · 2026-05-03T12:15:43Z

lili asked me to raise a PR for cmd += f" --size={size_gb}M" into cmd += f" --size={size_gb}G" . This requires more testing as this is going to impact multiple testcases. They all need to be tested and may need changes related to fio file sizes. Its needed to ensure that the fiofile size changes are not impacting the performance numbers and test execution times as well. This will be an elaborated exercise instead of simple type fix as I did in this PR. Hence closing this PR and I will raise the new PR when that is ready.

Copilot AI review requested due to automatic review settings April 6, 2026 16:11

SRIKKANTH requested review from LiliDeng, johnsongeorge-w and lubaihua33 as code owners April 6, 2026 16:11

Copilot started reviewing on behalf of SRIKKANTH April 6, 2026 16:12 View session

Copilot AI reviewed Apr 6, 2026

View reviewed changes

Copilot stopped work on behalf of SRIKKANTH due to an error April 6, 2026 16:47
Before you can use Copilot coding agent, you need to pick a "Usage billed to" option in your Copilot settings. Update your settings at https://github.com/settings/copilot/features, then try again.

Copilot AI review requested due to automatic review settings April 6, 2026 16:47

Copilot started reviewing on behalf of SRIKKANTH April 6, 2026 16:48 View session

Copilot AI reviewed Apr 6, 2026

View reviewed changes

Fixing incorrect variable name 'size_gb' which actually represents 's…

34396b8

…ize_mb' Fixing incorrect variable name 'size_gb' which actually represents 'size_mb' Co-Authored-By: Copilot <175728472+Copilot@users.noreply.github.com>

SRIKKANTH force-pushed the smyakam/26_04_05/size_gb_to_size_mb branch from c3afa4e to 34396b8 Compare April 6, 2026 17:25

kanchansenlaskar reviewed Apr 6, 2026

View reviewed changes

Comment thread lisa/microsoft/testsuites/cpu/cpusuite.py Outdated

Update cpusuite.py

eaab109

Copilot AI review requested due to automatic review settings April 6, 2026 17:36

Copilot started reviewing on behalf of SRIKKANTH April 6, 2026 17:37 View session

kanchansenlaskar approved these changes Apr 6, 2026

View reviewed changes

Copilot AI reviewed Apr 6, 2026

View reviewed changes

LiliDeng reviewed Apr 7, 2026

View reviewed changes

LiliDeng reviewed Apr 10, 2026

View reviewed changes

SRIKKANTH marked this pull request as draft April 14, 2026 04:58

SRIKKANTH closed this May 3, 2026

SRIKKANTH deleted the smyakam/26_04_05/size_gb_to_size_mb branch May 3, 2026 12:15

	size_mb=fio_data_size_in_gb * 1024,
	size_mb=fio_data_size_in_gb * 1024, # fio --size=<n>M uses MiB; convert GiB to MiB.

Conversation

SRIKKANTH commented Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Type of Change

Checklist

Test Validation

Test Results

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SRIKKANTH commented May 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SRIKKANTH commented Apr 6, 2026 •

edited

Loading